R4R: Store Refactor 1 #2985

mossid · 2018-12-03T15:07:58Z

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (docs/)
Added entries in PENDING.md with issue #
rereviewed Files changed in the github PR explorer

Addresses: #2986

For Admin Use:

Added appropriate labels to PR (ex. wip, ready-for-review, docs)
Reviewers Assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

codecov · 2018-12-03T15:16:07Z

Codecov Report

Merging #2985 into develop will decrease coverage by 0.07%.
The diff coverage is 64.07%.

@@             Coverage Diff             @@
##           develop    #2985      +/-   ##
===========================================
- Coverage    59.39%   59.32%   -0.08%     
===========================================
  Files          135      130       -5     
  Lines         9979     9809     -170     
===========================================
- Hits          5927     5819     -108     
+ Misses        3680     3620      -60     
+ Partials       372      370       -2

jaekwon · 2018-12-09T07:37:44Z

Will there be any logic changes in this PR? I hope not. Please hold off merging btw, I want to understand it, and need to plan ahead toward an KeyObjectStore wrapper for the KVStore.

mossid · 2018-12-10T16:39:03Z

@jaekwon There is no, most of the changes are just %sing. The most significant changes will be the removal of KVStore.Prefix() and KVStore.Gas() which are replaced by direct New{Prefix, Gas}Store calling, and types/{store, gas}.go moved into store/types/.

cwgoes

I think this package reorganization makes sense, lots of comments (some probably not introduced in this PR, but maybe we can fix them here...)

store/cachemulti/store.go

store/firstlast.go

store/iavl/wire.go

cwgoes · 2018-12-11T13:38:33Z

store/prefix/store_test.go

 )

+// copied from iavl/store_test.go
+var (
+	cacheSize        = 100


These should really be config parameters

Still true - could be a separate PR though.

store/queue/queue_test.go

store/types/store.go

cwgoes · 2018-12-11T13:42:01Z

store/types/store.go

+	// Start must be less than end, or the Iterator is invalid.
+	// Iterator must be closed by caller.
+	// To iterate over entire domain, use store.Iterator(nil, nil)
+	// CONTRACT: No writes may happen within a domain while an iterator exists over it.


fwiw we don't obey this contract at all (assuming writes include deletions, but even so) - we break it all over the codebase

is this actually required?

The iavl tree requires, but since we are cache wrapping the iavlstore before passing it to the handlers and the modification on the CacheKVStore is not reflected on the iterator, it looks safe to do it in the modules. It is still an exception but for the module writers' perspective we should update the comment to notify them that it is safe to modify in the modules

Reasonable - let's definitely update the comment

cwgoes · 2018-12-11T13:42:11Z

store/types/store.go

+	// Iterator over a domain of keys in descending order. End is exclusive.
+	// Start must be less than end, or the Iterator is invalid.
+	// Iterator must be closed by caller.
+	// CONTRACT: No writes may happen within a domain while an iterator exists over it.


same as above (we break the contract)

Update the comment here too.

store/types/store.go

jackzampolin · 2019-01-30T00:14:45Z

Would also be really nice to have a store/README.md that has brief (1 sentance is fine to start) descriptions of each of the different store impls and subpackages.

cwgoes · 2019-01-30T01:27:41Z

Needs linter fixes - and agreed with @jackzampolin.

alessio

Neat! Good to go from me.

Lint failures are unrelated, though I'm rerunning the build now.

…into joon/store-refactor-1

cwgoes · 2019-01-30T22:52:07Z

@mossid Is this WIP again now - are we waiting on something?

mossid · 2019-01-30T23:44:22Z

Working on the README, some parts are being written @cwgoes

cwgoes

A few notes on README.md - otherwise LGTM.

store/README.md

Co-Authored-By: mossid <[email protected]>

alexanderbez

There are a fair amount of changes here, but most of which look reasonable and are headed in a great direction. Approving.

jackzampolin

A great start at getting the /store package organized and documented! Good work @mossid ! LGTM

@jaekwon

Per @jaekwon

mossid added 4 commits December 3, 2018 11:45

in progress

a669428

store/* now compiles

83dac5d

make store/gas independent from types/, now pass ci

fe1209a

remove residue, rename files to match with convention

c40055e

mossid added the wip label Dec 3, 2018

mossid requested review from cwgoes, ebuchman, rigelrozanski and zramsay as code owners December 3, 2018 15:07

mossid changed the title ~~Store Refactor 1~~ WIP: Store Refactor 1 Dec 3, 2018

mossid added core Type: Code Hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity. labels Dec 3, 2018

fix lint

ff0e9dc

mossid mentioned this pull request Dec 3, 2018

Store Refactor #2986

Closed

mossid added 2 commits December 3, 2018 20:09

replace dependency types/ -> store/types/

b08044a

update pending

26e05ae

mossid force-pushed the joon/store-refactor-1 branch from 448c93a to 26e05ae Compare December 3, 2018 19:12

fix gas.NewStore argument order

324a849

mossid changed the title ~~WIP: Store Refactor 1~~ R4R: Store Refactor 1 Dec 3, 2018

mossid added ready-for-review and removed wip labels Dec 3, 2018

cwgoes self-assigned this Dec 4, 2018

mossid and others added 2 commits December 5, 2018 13:46

rename {gas, cache, trace} -> {gas, cache, trace}kv

9d806d6

Merge branch 'develop' into joon/store-refactor-1

cb8006b

jaekwon self-requested a review December 9, 2018 07:37

cwgoes suggested changes Dec 11, 2018

View reviewed changes

adress comments in progress

9876e30

address comments

e1f3fec

alessio approved these changes Jan 30, 2019

View reviewed changes

Merge branch 'develop' into joon/store-refactor-1

9b16f2f

alessio requested a review from jackzampolin January 30, 2019 18:51

mossid added 2 commits January 30, 2019 20:55

add README, fix lint, add AssertValidKey in prefix.Store

13470c3

Merge branch 'joon/store-refactor-1' of github.com:cosmos/cosmos-sdk …

997d88d

…into joon/store-refactor-1

mossid changed the title ~~R4R: Store Refactor 1~~ : Store Refactor 1 Jan 30, 2019

mossid changed the title ~~: Store Refactor 1~~ WIP: Store Refactor 1 Jan 30, 2019

mossid added wip and removed ready-for-review labels Jan 30, 2019

modify README

af69aeb

mossid changed the title ~~WIP: Store Refactor 1~~ : Store Refactor 1 Jan 30, 2019

mossid changed the title ~~: Store Refactor 1~~ R4R: Store Refactor 1 Jan 30, 2019

mossid added ready-for-review and removed wip labels Jan 30, 2019

cwgoes suggested changes Jan 31, 2019

View reviewed changes

store/README.md Outdated Show resolved Hide resolved

store/README.md Outdated Show resolved Hide resolved

store/README.md Outdated Show resolved Hide resolved

store/README.md Outdated Show resolved Hide resolved

store/README.md Outdated Show resolved Hide resolved

cwgoes and others added 3 commits January 30, 2019 16:25

Update store/README.md

7cd7880

Co-Authored-By: mossid <[email protected]>

Update store/README.md

ab82454

Co-Authored-By: mossid <[email protected]>

address comments

ae9eeb9

cwgoes approved these changes Jan 31, 2019

View reviewed changes

alexanderbez approved these changes Jan 31, 2019

View reviewed changes

jackzampolin approved these changes Jan 31, 2019

View reviewed changes

jackzampolin merged commit 08e62fb into develop Feb 2, 2019

ackratos mentioned this pull request Feb 11, 2019

expose versions field to support pruning cosmos/iavl#123

Closed

cwgoes deleted the joon/store-refactor-1 branch March 4, 2019 14:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

R4R: Store Refactor 1 #2985

R4R: Store Refactor 1 #2985

mossid commented Dec 3, 2018 •

edited

Loading

codecov bot commented Dec 3, 2018 •

edited

Loading

jaekwon commented Dec 9, 2018

mossid commented Dec 10, 2018

cwgoes left a comment

cwgoes Dec 11, 2018

alexanderbez Dec 20, 2018

cwgoes Jan 21, 2019

cwgoes Dec 11, 2018

mossid Dec 14, 2018

cwgoes Jan 11, 2019

cwgoes Dec 11, 2018

cwgoes Jan 11, 2019

jackzampolin commented Jan 30, 2019

cwgoes commented Jan 30, 2019 •

edited

Loading

alessio left a comment

cwgoes commented Jan 30, 2019

mossid commented Jan 30, 2019

cwgoes left a comment

alexanderbez left a comment

jackzampolin left a comment

R4R: Store Refactor 1 #2985

R4R: Store Refactor 1 #2985

Conversation

mossid commented Dec 3, 2018 • edited Loading

codecov bot commented Dec 3, 2018 • edited Loading

Codecov Report

jaekwon commented Dec 9, 2018

mossid commented Dec 10, 2018

cwgoes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackzampolin commented Jan 30, 2019

cwgoes commented Jan 30, 2019 • edited Loading

alessio left a comment

Choose a reason for hiding this comment

cwgoes commented Jan 30, 2019

mossid commented Jan 30, 2019

cwgoes left a comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

jackzampolin left a comment

Choose a reason for hiding this comment

mossid commented Dec 3, 2018 •

edited

Loading

codecov bot commented Dec 3, 2018 •

edited

Loading

cwgoes commented Jan 30, 2019 •

edited

Loading